Add support for UnsafeRawBufferPointer to DispatchData #230
Conversation
(Radar 30699743)
|
@swift-ci please test |
src/swift/Data.swift
Outdated
| /// Initialize a `Data` without copying the bytes. | ||
| /// | ||
| /// - parameter bytes: A buffer pointer containing the data. | ||
| /// - parameter deallocator: Specifies the mechanism to free the indicated buffer. | ||
| @available(swift, deprecated: 4, message: "Use init(bytes: UnsafeRawBufferPointer, deallocater: Deallocator) instead") |
There was a problem hiding this comment.
Is this supposed to say "bytesNoCopy"?
There was a problem hiding this comment.
Yes, good catch.
src/swift/Data.swift
Outdated
| /// | ||
| /// - parameter pointer: A pointer to the buffer you wish to copy the bytes into. | ||
| /// - parameter count: The number of bytes to copy. | ||
| /// - warning: This method does not verify that the contents at pointer have enough space to hold `count` bytes. |
There was a problem hiding this comment.
Maybe it should do that, now that you're passing a buffer pointer?
There was a problem hiding this comment.
Stepping back, does it also make sense to default the count to the entire data, or would someone just not write that? How do I do an offset?
Stepping back even further…is this a necessary API at all? Normally in Swift we'd write this as
buffer.append(data[0..<count])
and if that's not going to be as efficient we should try to figure out what to do there. cc @airspeedswift
There was a problem hiding this comment.
Maybe it should do that, now that you're passing a buffer pointer?
Yes, I think it should probably copy min(buffer count, given count) [and that behavior will be documented].
Regarding the other comments, this change is intended to be the minimal change to allow us to support UnsafeRawBufferPointer. We want to keep the API in line with that of Foundation's Data and to that end I don't want to add anything new until Foundation has agreed to do the same. There will be more work in this area in the future.
There was a problem hiding this comment.
How do you know if it copied the whole thing? I'd prefer to assert, so that clients are encouraged to check the length beforehand.
There was a problem hiding this comment.
This appears to duplicate the role of UnsafeMutableRawBufferPointer.initializeMemory<S: Sequence>(as: S.Iterator.Element.Type, from source: S). There shouldn't be a performance penalty if we do the _copyContents trick (which it looks like UnsafeMutableRawBufferPointer isn't taking advantage of, but should). We should probably revisit this. cc @phausler
There was a problem hiding this comment.
I'd prefer to assert, so that clients are encouraged to check the length beforehand.
That seems like a good idea.
There was a problem hiding this comment.
Stepping back even further…is this a necessary API at all? Normally in Swift we'd write this as
buffer.append(data[0..<count])
This kind of thing is not likely to be efficient because the DispatchData could be composed of several discontiguous regions which would have to first be flattened (by copy) before being sliced and copied again (unless I am misunderstanding how that statement would work). The existing copy code handles the discontinuity without additional copies.
There was a problem hiding this comment.
Slicing doesn't immediately flatten, but append might go byte-by-byte by default. Ben would know more than me, though.
|
What about |
It's a deliberate choice to not add support for UnsafeRawBufferPointer to the enumerateBytes and withUnsafeBytes methods at this time. Adding a variant of either method that accepts an UnsafeRawBufferPointer argument creates an ambiguity at the call site when writing the closure, unless the user explicitly writes the type of the buffer argument. In a later release, we plan to add better APIs to both the DispatchData and the Foundation Data classes. |
|
@swift-ci please test |
|
Updated for review comments. The API-related comments are deferred until later, pending further discussion with Foundation. |
|
@swift-ci please test |
|
shouldnt you also fix this too? |
(Radar 30699743)
rdar://problem/30699743